-
Notifications
You must be signed in to change notification settings - Fork 42
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
webExtension module #778
base: main
Are you sure you want to change the base?
webExtension module #778
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR! Quite a few comments, but this does look like it's heading in the right direction, so don't be put off :)
index.bs
Outdated
<pre class="cddl remote-cddl local-cddl"> | ||
ExtensionDetails = { | ||
extension: Extension, | ||
? installTemporary: bool .default true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we might be better off only supporting temporary installs at first. I don't really have an automation use case in mind where it makes sense to permanently install an extension in a specific profile, and we can always add it if there is one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jgraham would a temporary install be something new defined by the webdriver bidi spec or do you already have a notion of "temporary" extensions in Firefox (I guess they auto-uninstall)?
@oliverdunk do we have something like this within extensions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think having this set to false
would be the current behavior - unpacked extensions stay installed between restarts as long as you use the same profile. I can imagine how true
might work but I'm not sure I'm convinced of the value of that, at least for the MVP.
index.bs
Outdated
1. Let the |extension| be the value of the <code>extension</code> field of | ||
|command parameters|. | ||
|
||
1. [=Try=] to install web-extension with |session| and |extension|. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So there's quite a few bits missing here:
- It's possible this command isn't supported, so you need to say something like "If installing |extension| isn't supported for any reason return error with error code unsupported operation".
- You need to explictly deal with the different input types, so that end up with some abstract representation of the set of files that make up the extension.
- It would probably be good to make some checks explicit here. For example if there's no
manifest.json
file in the root that should return an error. - You need to handle the other data passed in e.g. the private browsing mode flag.
- You need a bit more detail on what it means to install a web extension. It's going to be "implementation defined steps", but it should probably describe what the outcome is supposed to be. This will still need to be entirely fallible (i.e. there's a possibility of the install failing for implemenation-specific reasons), as in the current text.
FYI: There is prior context in the WECG to discuss the desired characteristics of extension loading functionality. For reference, here are the meeting notes:
Related issues: |
@jgraham thank you very much for the review. I will need a moment to unpack all comments, but sure to receive progress soon. |
Hi Krzysztof. I wanted to check back if you already had some time to distill the feedback from James. If not no worries, but let us know in case you don't have the time to continue on this PR. Thanks! |
index.bs
Outdated
1. Let |path| be a value of <code>path</code> field of |install path spec| if the value of <code>type</code> of the |install path spec| is equal to <code>path</code>, | ||
otherwise perform any implementation-defined steps to create a temporary folder and make its path to be the value of |path|. | ||
|
||
1. Let |archive path| be a value of <code>path</code> field of |install path spec| if the value of <code>type</code> of the |install path spec| is equal to <code>archive-path</code>, | ||
otherwise if the value of <code>type</code> of the |install path spec| is equal to <code>archive-path</code> perform any implementation-defined steps to create a temporary file | ||
being a result of Base64 decodeing of the value of <code>value</code> filed of |install path spec| and make its path to be the value of |archive path|, | ||
otherwise |archive path| is null. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do realize this is overly complicated, but wasn't sure how to express it otherwise. Please advise.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm confused by the flow of this, since each step has something along the lines of "If A do this, otherwise if A do that". I would have expected something like:
- If the type is "path", return the path.
- Otherwise, if the type is "archive-path", extract the archive and return the path to the temporary directory.
- Otherwise, if the type is "base64", decode this as a zip and return the path to the temporary directory.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it better now? or you would rather remove the intermediate data structure?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The extraction process is much clearer now, thanks - I'm happy with that as long as we are comfortable having a switch vs a series of if/else if (cc/ @jgraham). I'm still a little unclear why we need |extension files| and |extension archive|. Is there a reason for that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this! It's really great to see an external contribution and it lines up nicely with our goal in the WebExtensions Community Group to write cross-browser tests for extension APIs.
I've left some feedback but please prioritize addressing anything from @jgraham who is the one with spec experience :)
index.bs
Outdated
|
||
1. Let |extension files| be a value of <code>files<code> field of |extension archive|. | ||
|
||
1. If |extension files| does not include entry <code>manifest.json</code> return [=error=] with code [=invalid extension=] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would lean towards dropping this, and in the subsequent step having something that allows the browser to return an error if anything goes wrong. There are many ways an extension can be invalid beyond the manifest.json not being present (e.g an unsupported manifest_version
) and I don't think we want to list them all here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we could delete it; as you say there are many failure cases and we can't enumerate them all. On the other hand, it's also clear that if there isn't a manifest.json
at all the install must fail, so assuming it's not hard to specify I think checking the file exists is reasonable, and makes the spec more precise (by defining at least one case where failure is mandatory).
index.bs
Outdated
1. Let |path| be a value of <code>path</code> field of |install path spec| if the value of <code>type</code> of the |install path spec| is equal to <code>path</code>, | ||
otherwise perform any implementation-defined steps to create a temporary folder and make its path to be the value of |path|. | ||
|
||
1. Let |archive path| be a value of <code>path</code> field of |install path spec| if the value of <code>type</code> of the |install path spec| is equal to <code>archive-path</code>, | ||
otherwise if the value of <code>type</code> of the |install path spec| is equal to <code>archive-path</code> perform any implementation-defined steps to create a temporary file | ||
being a result of Base64 decodeing of the value of <code>value</code> filed of |install path spec| and make its path to be the value of |archive path|, | ||
otherwise |archive path| is null. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm confused by the flow of this, since each step has something along the lines of "If A do this, otherwise if A do that". I would have expected something like:
- If the type is "path", return the path.
- Otherwise, if the type is "archive-path", extract the archive and return the path to the temporary directory.
- Otherwise, if the type is "base64", decode this as a zip and return the path to the temporary directory.
@oliverdunk thank you for all the suggestions! Happy to contribute to WECG goals. |
index.bs
Outdated
<pre class="cddl remote-cddl local-cddl"> | ||
extensions.Extension = { | ||
installPath: extensions.ExtensionPath / extensions.ExtensionArchivePath / extensions.ExtensionBase64Encoded, | ||
? allowPrivateBrowsing: bool .default false, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@oliverdunk do you know if we have an equivalent of what allowPrivateBrowsing is expressing here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, thanks! I wonder if alternatively we could specify a user context ID to install an extension to? (I would need to check if it's implementable though)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't believe that per-user-context containers would be possible in the current Firefox implementation. I'd propose dropping this from the initial proposal since we don't really have the concept of private browsing exposed elsewhere in the specification.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we then rename the property to temporary
to indicate that the extension will not be installed permanently? That also uses a name that is not tied too much to a given browser.
Btw. we also use this name for our current implementation for WebDriver classic in Firefox.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually my last answer was wrong. Temporary actually means that the web extension is not persistent and can be installed even when it isn't signed (like for developers), and that is indeed a feature that we support in Firefox.
But we do not yet support the private browsing
mode installation. So both things are different.
@OrKoN How does Chrome handle the installation of unsigned web extensions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In Chrome, you enable "Developer Mode" with a toggle on the chrome://extensions page and then load a directory or zip file with a "Load unpacked" button. Unlike Firefox, we don't couple the concept of unsigned and temporary extensions. An unsigned extension will always persist until it is removed - just like with a signed extension.
Given that, I would be against having a temporary
property which is required for loading an unpacked extension. That is coupling two unrelated concepts.
I believe as implemented, the Extensions.loadUnpacked
CDP command would fail for a packed extension.
Given that, I wonder if we could do one of a few options:
- Scope the MVP to just unpacked extensions. We wouldn't need any additional flags.
- Have the browser autodetect packed or unpacked. We also wouldn't need any additional flags.
- Have a flag that is specifically named to decide which path we go down trying to load the extension.
For all of this, I think whether this ends up being temporary or not can be for the user-agent to decide. Let me know if I'm wrong but I don't think there is much presidence for tests that span across browser sessions? If you care about having a clean state you should be creating a new profile directory.
index.bs
Outdated
|
||
<pre class="cddl remote-cddl local-cddl"> | ||
extensions.Extension = { | ||
installPath: extensions.ExtensionPath / extensions.ExtensionArchivePath / extensions.ExtensionBase64Encoded, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In Chrome, we currently only allow installing extensions if the client is local to the browser (i.e., connected via pipes instead of the WebSocket). We could probably relax this based on a command line argument but I wonder if it makes sense to start with the extensions.ExtensionPath only?
cc @oliverdunk for additional thoughts
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could that be managed by the webdriver? Archives (zip/crx) and Base64 strings are can effectively be unpacked to a temporary folder, and then run as from any other path.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the driver process and the browser might not be running on the same machine so, in a general case, we probably should not handle it in the driver. I think a remote-end path to an archive or to an unpacked folder is less of a concern but installing an extension from base64 would mean writing something to the filesystem that was not present there originally.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@OrKoN, I'm fairly certain Selenium currently supports loading an extension from base64 in WebDriver. Do you have any idea how that was implemented?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
base64 is probably the only useful format if you're trying to install things on a remote browser; in that case you might well want to specifically disallow access to filesystem paths.
It should certainly be allowed for implementations to return Unsupported operation
for any variant they don't support here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@OrKoN, I'm fairly certain Selenium currently supports loading an extension from base64 in WebDriver. Do you have any idea how that was implemented?
they probably decode it and unpack it to the local filesystem and provide the path as the launch arg for Chrome. I don't think they use the CDP method to install extensions at runtime.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we have any concerns that base64-encoded extensions could be too large for transporting as JSON over the protocol? I do not know how large is a typical extension bundle.
I think extension installation here is similar to input.setFiles which currently also supports only remote-end absolute paths.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Extensions can be quite big. I think AdblockPlus is around 60mb while Ghostery is around 12mb.
index.bs
Outdated
<pre class="cddl remote-cddl local-cddl"> | ||
ExtensionDetails = { | ||
extension: Extension, | ||
? installTemporary: bool .default true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jgraham would a temporary install be something new defined by the webdriver bidi spec or do you already have a notion of "temporary" extensions in Firefox (I guess they auto-uninstall)?
@oliverdunk do we have something like this within extensions?
My plan was to join the group and I've made a request to W3C. In result I was added to WECG group, but not to webdriver. Not sure whom to ask fix it. Can you recommend a contact? |
re #778 (comment) |
thanks for details so it's different from unpacked extensions in Chrome because they persist in the profile. |
I think this link should lead to a form to join the browser testing and tools working group https://www.w3.org/groups/wg/browser-tools-testing/join |
index.bs
Outdated
|
||
1. Let |extension files| be a value of <code>files<code> field of |extension archive|. | ||
|
||
1. If |extension files| does not include entry <code>manifest.json</code> return [=error=] with code [=invalid extension=] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we could delete it; as you say there are many failure cases and we can't enumerate them all. On the other hand, it's also clear that if there isn't a manifest.json
at all the install must fail, so assuming it's not hard to specify I think checking the file exists is reasonable, and makes the spec more precise (by defining at least one case where failure is mandatory).
index.bs
Outdated
|
||
<pre class="cddl remote-cddl local-cddl"> | ||
extensions.Extension = { | ||
installPath: extensions.ExtensionPath / extensions.ExtensionArchivePath / extensions.ExtensionBase64Encoded, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
base64 is probably the only useful format if you're trying to install things on a remote browser; in that case you might well want to specifically disallow access to filesystem paths.
It should certainly be allowed for implementations to return Unsupported operation
for any variant they don't support here.
index.bs
Outdated
<dt>"<code>archive-path</code>" | ||
<dd> | ||
1. Let |archive path| be a value of <code>path</code> field of |install path spec|. | ||
1. Perform any implementation-defined steps to extract the archive from <code>archive path</code> to a temporary directory and let |path| be the path to that temporary directory. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This definitely doesn't have to require a temporary directory (that is: doing this entirely in-memory should be permitted). Also this seems to allow an unbounded range of archive formats, whereas I guess we only actually want to support zip? Zip isn't well-specified afaik, so actually talking about how to decode that might be a problem (but it needs to be falliable, in case the file is invalid).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not quite sure how to achieve that. Can you please check the latest version to verify if I go into right direction?
608978e
to
da00aa5
Compare
be3fb96
to
5a87256
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you, @chrmod, for all your hard work! It's looking great, but there are a few items I'd like to discuss and see addressed. All of them can be found inline.
index.bs
Outdated
|
||
1. Let |extension data spec| be a value of <code>extensionData</code> field of the |command parameters|. | ||
|
||
1. Let |extension directory entry| be the result of [=trying=] to [=expand a extension data spec=] with |extension data spec|. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should the directory entry
part reference the other spec?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isn't it enough if |expand a extension data spec|
returns a [=directory entry=]
?
Co-authored-by: Henrik Skupin <[email protected]>
Co-authored-by: Henrik Skupin <[email protected]>
Co-authored-by: Henrik Skupin <[email protected]>
Co-authored-by: Henrik Skupin <[email protected]>
Co-authored-by: Alex Rudenko <[email protected]>
Co-authored-by: kiaraarose <[email protected]>
Co-authored-by: Alex Rudenko <[email protected]>
Co-authored-by: Henrik Skupin <[email protected]>
Co-authored-by: Henrik Skupin <[email protected]>
Co-authored-by: jgraham <[email protected]>
ebec014
to
edfa91f
Compare
@whimboo done and tests are green now. |
@jgraham can you please take a look? |
addresses #548
We aim to provide a capability and a command to install a web-extension in the session.
This is a pre-requisite to allow WedDriver Bidi to interact with the extension background pages and service workers.
Preview | Diff